What

In Protocol._requestWithSchema (protocol.ts:363) the refactored code calls:

this.assertCapabilityForMethod(request.method as RequestMethod);

and in Protocol.notification (protocol.ts:379):

this.assertNotificationCapability(notification.method as NotificationMethod);

Both abstract methods are declared (protocol.ts:113, :120) as:

protected abstract assertCapabilityForMethod(method: RequestMethod | string): void;
protected abstract assertNotificationCapability(method: NotificationMethod | string): void;

Since RequestMethod and NotificationMethod are string-literal unions, RequestMethod | string and NotificationMethod | string both collapse to string. Meanwhile Request.method and Notification.method are typed as plain string (spec.types.ts). The casts therefore add nothing for compilation and unsafely assert that an arbitrary method string is a member of the spec literal union — which is false for custom (non-spec) methods that this API explicitly supports.

Step-by-step

1. request: Request arrives with request.method: string — e.g. a custom method 'acme/search'.
2. The PR casts it to RequestMethod, claiming it is one of 'ping' | 'tools/list' | .... That claim is untrue.
3. assertCapabilityForMethod's parameter type is RequestMethod | string ≡ string, so the call would have type-checked without the cast.
4. Result: the cast is a pure narrowing lie with zero compilation benefit.

Regression vs. pre-refactor

The diff's removed lines show the previous implementation called this.assertCapabilityForMethod(request.method) and this.assertNotificationCapability(notification.method) with no cast and compiled fine. This PR's stated goal is behavior parity for the public surface; introducing two unjustified as assertions is a small code-quality regression. REVIEW.md's checklist explicitly calls out "Type safety: no unjustified any, no unsafe as assertions".

Why it's worth fixing despite zero runtime impact

One verifier argued this is too trivial to report because the parameter immediately re-widens to string, so no type hole is exploited. That's true for runtime — there is no behavioral bug. But the concern isn't a runtime hole; it's that (a) the cast is dead weight, (b) it sets a misleading precedent that request.method is a RequestMethod at this point (it isn't — custom methods flow through here), and (c) it's a regression vs. the code being replaced. In a parity-focused refactor PR, gratuitous new as casts are exactly the kind of drift worth catching. The fix is a two-token deletion, so the cost/benefit clearly favors mentioning it as a nit.

Fix

this.assertCapabilityForMethod(request.method);
// ...
this.assertNotificationCapability(notification.method);

What's inconsistent

Protocol.use(mw: DispatchMiddleware) (protocol.ts:156) and Protocol.dispatch(req, env?: RequestEnv): AsyncGenerator<DispatchOutput> (protocol.ts:166) are new methods with full user-facing JSDoc and no @internal tag. The PR title literally says "dispatch() public". But every type in their signatures carries @internal:

• DispatchMiddleware — dispatcher.ts:46 @internal
• DispatchOutput — dispatcher.ts:27 @internal
• DispatchFn — dispatcher.ts:36 @internal
• RequestEnv — context.ts:236 @internal
• the Dispatcher class itself — dispatcher.ts:88 @internal

So we have public-visibility methods whose parameter and return types are documented as not-for-external-use.

Why it surfaces to end users (addressing the "Protocol isn't public anyway" objection)

It's true that packages/core/src/exports/public/index.ts:41 deliberately omits the Protocol class from the curated public surface, and @modelcontextprotocol/core is private: true. But that doesn't fully neutralise this: Client extends Protocol<ClientContext> (client.ts:221) and Server extends Protocol<ServerContext> are the end-user-facing classes, and they inherit use() / dispatch(). A consumer of @modelcontextprotocol/client will see client.use(...) and client.dispatch(...) in IntelliSense with full JSDoc, and can call them — the curated core/public re-export list doesn't gate inherited instance methods. So the question "is this meant for end users?" is live regardless of whether Protocol itself is re-exported.

What does not break (addressing the over-stated harm)

The repo has no stripInternal in any tsconfig, no api-extractor config, and typedoc.config.mjs does not set excludeInternal (default false). @internal here is a documentation convention, not a build directive. The types are also runtime-exported via the core barrel (index.ts → export * from './shared/dispatcher.js'; protocol.ts → export * from './context.js'). So generated .d.ts will not reference a missing symbol, and TypeDoc will not produce a dangling link. This is why it's a nit, not a bug.

Concrete walk-through

1. End user installs @modelcontextprotocol/server and writes const s = new Server(...).
2. Autocomplete on s. shows use and dispatch with the JSDoc "Registers a DispatchMiddleware around the inbound request path" / "Transport-free entry point".
3. User goes to write s.use(next => ...) — the parameter type is DispatchMiddleware, whose own JSDoc says @internal. Same for for await (const out of s.dispatch(req)) → out: DispatchOutput is @internal.
4. Nothing fails — but the user can't tell whether this is supported API or an implementation detail they shouldn't touch. Meanwhile the PR description says the actual consumers are handleHttp, tasksPlugin, RequiresInput, ContinuationCompat — all in-monorepo.

Suggested resolution

Pick one direction and make the annotations agree:

• If SDK-internal seam (matches the PR description's stated consumers): add @internal to the JSDoc on use() and dispatch() so they match their parameter types. The "public" in the PR title then just means "public class-member visibility for sibling packages", which is fine.
• If end-user API: drop @internal from DispatchMiddleware / DispatchOutput / DispatchFn / RequestEnv and add them to packages/core/src/exports/public/index.ts deliberately, per CLAUDE.md § Public API Exports / REVIEW.md "Every new export is intentional".

Either is a one-line doc change; the point is to state intent so later PRs in the stack don't have to guess.

What

CLAUDE.md is the repo-level contributor architecture doc (and the file Claude itself loads for repo context). Its Architecture / Message Flow section currently reads:

• Line 157: "4. Protocol._onresponse() resolves the promise when response arrives"
• Line 164: "2. Protocol.connect() routes to _onrequest(), _onresponse(), or _onnotification()"
• Lines 165–166: "3. Protocol._onrequest(): Looks up handler in _requestHandlers map"

After this PR, none of those private members exist on Protocol. _onrequest / _onresponse / _onnotification and the message-routing in onmessage now live in StreamDriver (packages/core/src/shared/streamDriver.ts); _requestHandlers lives in Dispatcher (packages/core/src/shared/dispatcher.ts); and Protocol.connect() no longer routes messages itself — it constructs a StreamDriver and calls driver.start().

Why this PR

R1 (1916e69) only added Dispatcher/StreamDriver alongside the old code. R2 (this PR, 62d993f) is the commit that actually removes _onrequest/_onresponse/_onnotification/_requestHandlers from Protocol and rewires connect(). So this is the PR that turns CLAUDE.md's prose from accurate into stale. The PR adds a correct file-header at protocol.ts:1-7 describing the new composition, but CLAUDE.md was not updated to match — it's not in the changed-files list.

Why it matters

REVIEW.md's checklist says: "Bugfix or behavior change: check whether docs/**/*.md describes the old behavior and needs updating; flag prose that now contradicts the implementation." CLAUDE.md is at the repo root rather than under docs/, but it's the primary contributor-facing architecture doc and is loaded into Claude's context for this repo, so the same principle applies. A new contributor following the Message Flow section will grep for Protocol._onrequest and find nothing.

Step-by-step proof

1. grep -n "_onrequest\|_onresponse\|_requestHandlers" packages/core/src/shared/protocol.ts → no hits after this PR (protocol.ts is 393 lines, none of these identifiers appear).
2. grep -n "_onrequest\|_onresponse\|_onnotification" packages/core/src/shared/streamDriver.ts → all three present (private methods on StreamDriver).
3. grep -n "_requestHandlers" packages/core/src/shared/dispatcher.ts → protected _requestHandlers: Map<...> on Dispatcher.
4. sed -n '155,170p' CLAUDE.md → still references Protocol._onresponse(), Protocol._onrequest(), _requestHandlers map on Protocol.
5. PR changed-files list: CLAUDE.md is absent.

Impact

No runtime effect — purely doc drift on internal architecture. Hence nit.

Fix

Update CLAUDE.md's Message Flow section to reflect the new composition, e.g.:

• Outbound: Protocol.request() → StreamDriver.request() (assigns id, registers response handler, sets timeout) → Transport.send(); StreamDriver._onresponse() resolves the promise.
• Inbound: Transport.onmessage → StreamDriver routes to _onrequest() / _onresponse() / _onnotification(); _onrequest() calls Dispatcher.dispatch(), which looks up the handler in Dispatcher._requestHandlers and yields notifications + a terminal response that StreamDriver writes back to the pipe.

Fine to land in this PR or as a follow-up in the same stack.